add BigIntegerConverter#308
Conversation
|
@arnzel Thank you for your contribution. As we try to improve our test coverage we require all PR-s to include tests. Can you please add some and update this PR? |
|
yes sure |
|
@zapodot : i added some tests and library assertJ for better failing errors |
|
@arnzel I see from your commit that you have added a dependency on AssertJ. Any chance you could use the basic JUnit assertions instead? Thanks again for you contribution 👍 |
|
|
||
| import java.math.BigInteger; | ||
|
|
||
| public class BigIntegerConverterTest { |
There was a problem hiding this comment.
Could you also add a test where you try to convert a null value?
well i can do it,but i dont like JUNIt Assertions like "assertEquals" and "assertTrue". they only tell you somethig like "expected true but was false".AssertJ gives you much more information and is better maintained like other matching libraries like hamcrest |
|
There is an assertThat assertion in JUnit as well if you prefer that :-)
That does not mean that we will not consider assertj at some point or more likely move to JUnit 5 to support a richer set of assertions.
…On Wed, Nov 21, 2018 at 2:07 PM Arne Zelasko ***@***.***> wrote:
@arnzel <https://github.com/arnzel> I see from your commit that you have
added a dependency on AssertJ. Any chance you could use the basic JUnit
assertions instead? Thanks again for you contribution
well i can do it,but i dont like JUNIt Assertions like "assertEquals" and
"assertTrue". they only tell you somethig like "expected true but was
false".AssertJ gives you much more information and is better maintained
like other matching libraries like hamcrest
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#308 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAhqkasIBRyEl47p9tIuTPIsB3cZRrLMks5uxVAEgaJpZM4YmW7e>
.
|
|
@zapodot okay i removed assertJ and use hamcrest matchers. also i added converting null values |
|
@zapodot : i dont understand travic ci test error, is it related to my change ? |
|
@arnzel we are experiencing some weird behaviour in our builds right now. Will await merge before until we have a green master branch. Good job! |
nryanov
left a comment
nryanov
left a comment
There was a problem hiding this comment.
It seems that in cases when we want to get BigInteger instance from Number or String we need to use number.longValue() or Long.parseLong(str) to save precision, otherwise, if number exceeds the MAX_INT then after conversion we'll lose initial value
Thank you for your work!
| return (BigInteger)number; | ||
| } | ||
| else{ | ||
| return BigInteger.valueOf(number.intValue()); |
There was a problem hiding this comment.
Is seems that number.longValue() would be better than number.intValue(). Otherwise you can lost precision.
Also, it's more native to use long than int when you want to get BigInteger
| @Override | ||
| protected BigInteger convertStringValue(String string) { | ||
| if(null != string) { | ||
| return BigInteger.valueOf(Integer.parseInt(string)); |
There was a problem hiding this comment.
Same here. Long.parseLong(...) would be better or, maybe, there is another even better option?
3 participants
Add Converter for BigInteger